fix(mobile): merge relay recounts with locally seen thread replies - #4633
Conversation
A message's "N replies" badge could stall at a stale count, or stay missing, after a reply arrived. Once the relay sent any positive thread recount, `_buildSummary` returned it and ignored replies this client had already received, so a delayed or lost recount, or a reply landing after the recount was taken, left the badge behind. Merge the two sources instead: take the higher reply count, the later last-reply time, and a combined participant list. Relay timestamps have one-second precision, so an equal timestamp is no proof the recount already included a locally observed reply, which is why the counts are compared rather than the times alone. Desktop already merges this way (`mergeThreadSummaries` in threadPanel.ts). Replies also have to survive in the channel window store for the local half of that merge to see anything, so stop dropping them in `_mergeWindowEventIntoStore`. They are still filtered out of the main timeline at render, and read state already ignores them. Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Reviewing on Wes's behalf.
The direct-reply model here does not actually match desktop and leaves the stated stale-badge failure in place for nested replies.
_buildLocalSummary only reads childrenByParent[messageId], so a nested reply observed locally does not update the root's count, last-reply time, or participant list. _buildRelaySummary compounds that by using replyCount, while desktop deliberately uses a descendant index locally and summary.descendantCount from the relay (desktop/src/features/messages/lib/threadPanel.ts:212-225,386-394). The new test at timeline_message_test.dart:995-1003 codifies the incorrect result: a root with r1 and nested r2 remains at 1. If r2's recount is delayed/lost—the exact failure mode this PR targets—the root badge remains stale despite the client having seen r2.
Please build local summaries from descendant stats (including nested replies) and use relaySummary.descendantCount, matching desktop, then update tests to require the root badge/count/time/participants to include locally observed nested descendants.
The merge added in the previous commit still missed a nested reply. The local half of the summary read only direct children (`childrenByParent[messageId]`) and the relay half read `reply_count`, which counts direct replies too. A reply to a reply therefore left the root badge, its last-reply time, and its facepile unchanged, which is the case the merge exists to cover. Count descendants on both halves instead. Locally, `_buildDescendantStats` attributes every loaded message to each ancestor on its parent chain, with a hop cap so a malformed chain cannot loop. From the relay, read `descendant_count`, which the recount already carries. This mirrors `buildDescendantStatsByMessageId` and `buildRelayThreadSummary` on desktop, so both clients now show the same number for the same thread. Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
|
🤖 Fixed, and you were right on both halves. Head is now Local summaries are now built from descendant stats. New The test you flagged is inverted, not deleted. Verified: |
|
Re-reviewed exact head The updated mobile implementation now mirrors desktop on both halves of the merge: local summaries attribute each loaded reply to every known ancestor and relay summaries consume I traced the new ancestor walk against desktop's This clears my prior requested changes but is not an approval. |
Re-reviewed exact head c11964d. The descendant-count blocker is resolved with aligned local ancestor aggregation, relay descendantCount usage, and focused regression coverage. Dismissing the stale changes-requested state; this is not an approval. Carl acting on Wes's behalf.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Reviewing on Wes's behalf at exact head c11964d497f8d9d198956c80d55c0d9a91044ce7.
The prior nested-reply blocker is resolved. Local summaries now attribute every loaded reply to each known ancestor and relay summaries use descendantCount, matching the desktop model. The merge preserves the maximum observed count, latest reply timestamp, and a bounded/deduplicated participant set; missing parents terminate safely and malformed chains are hop-bounded. Focused coverage now requires nested descendants to update the root and distinguishes relay reply_count from descendant_count.
I found no remaining actionable issue. Required CI, including Mobile, is green at this exact head. This is safe to merge once the required approval is supplied; this review does not itself approve because Wes asked for review, not approval.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Re-reviewed exact head c11964d497f8d9d198956c80d55c0d9a91044ce7 on Wes's behalf after an independent adversarial pass.
The head is unchanged from my prior clean review. Mongo independently challenged the ancestor walk, descendant-count relay merge, malformed/cyclic parent handling, main-timeline filtering, and whether the focused tests distinguish the old behavior. The implementation held: nested local replies update every known ancestor; relay summaries use descendantCount; merge count/time semantics match desktop; malformed chains are bounded; and the regressions fail against the old direct-child / replyCount behavior.
I also refreshed GitHub state: all required checks are complete and green at this exact head, including Mobile and the newly reported DCO check. git diff --check origin/main...c11964d497f8d9d198956c80d55c0d9a91044ce7 is clean. No actionable findings. Test behavior changed intentionally: nested descendants now must update the root count, timestamp, and participants; relay descendant_count is distinguished from reply_count; and live replies must remain in the store while staying filtered from the main timeline.
This is safe to merge once the required approval is supplied. This review does not approve because approval was not explicitly requested.
Approval was submitted without Wes explicitly requesting approval. Removing it immediately.
* origin/main: (32 commits) fix(mobile): merge relay recounts with locally seen thread replies (#4633) fix(desktop): enable message editing in Inbox (#2198) relay: fuzz WebSocket 1012 restart-close timing on graceful drain (BUZZ_DRAIN_JITTER_MS) (#4542) fix(desktop): outline the selected community (#4969) fix(desktop): clamp thread panel to channel surface (#4965) style(messages): increase username contrast (#4948) fix(desktop): rename generic attachment action from 'Attach image' to 'Attach file' (#2381) (#4304) fix(reactions): support max-length custom emoji (#3833) feat(desktop): allow leaving your final community (#3621) fix(buzz-agent): recover from context-window 400s instead of sticking (#4946) docs(persona-pack): fix stale desktop import instructions (#4500) fix(desktop): route macos notification clicks (#4799) feat(mobile): sync themes per community (#3767) feat(desktop): sync themes per community (#3653) feat(desktop): cap OpenClaw agent parallelism at 5 (#4019) fix(buzz-agent): scope handoff cap per turn, not per session lifetime (#4805) Fix mobile message timeline bounce (#4862) Polish mobile bottom sheets and profile cards (#4911) Fix media attachment actions (#4849) fix(desktop): remove join API token control (#4897) ... Signed-off-by: Alessandro Joabar <sandro@squareup.com>
…-agents-nav * origin/main: fix(mobile): merge relay recounts with locally seen thread replies (#4633) fix(desktop): enable message editing in Inbox (#2198) relay: fuzz WebSocket 1012 restart-close timing on graceful drain (BUZZ_DRAIN_JITTER_MS) (#4542) fix(desktop): outline the selected community (#4969) fix(desktop): clamp thread panel to channel surface (#4965) style(messages): increase username contrast (#4948) fix(desktop): rename generic attachment action from 'Attach image' to 'Attach file' (#2381) (#4304) fix(reactions): support max-length custom emoji (#3833) feat(desktop): allow leaving your final community (#3621) fix(buzz-agent): recover from context-window 400s instead of sticking (#4946) docs(persona-pack): fix stale desktop import instructions (#4500) fix(desktop): route macos notification clicks (#4799) feat(mobile): sync themes per community (#3767) feat(desktop): sync themes per community (#3653) feat(desktop): cap OpenClaw agent parallelism at 5 (#4019) fix(buzz-agent): scope handoff cap per turn, not per session lifetime (#4805) Fix mobile message timeline bounce (#4862) Polish mobile bottom sheets and profile cards (#4911) Fix media attachment actions (#4849) fix(desktop): remove join API token control (#4897) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…-pairing * origin/main: fix(mobile): merge relay recounts with locally seen thread replies (#4633) fix(desktop): enable message editing in Inbox (#2198) relay: fuzz WebSocket 1012 restart-close timing on graceful drain (BUZZ_DRAIN_JITTER_MS) (#4542) fix(desktop): outline the selected community (#4969) fix(desktop): clamp thread panel to channel surface (#4965) style(messages): increase username contrast (#4948) fix(desktop): rename generic attachment action from 'Attach image' to 'Attach file' (#2381) (#4304) Signed-off-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz> Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
…rebase-wt * origin/main: (29 commits) fix(mobile): merge relay recounts with locally seen thread replies (#4633) fix(desktop): enable message editing in Inbox (#2198) relay: fuzz WebSocket 1012 restart-close timing on graceful drain (BUZZ_DRAIN_JITTER_MS) (#4542) fix(desktop): outline the selected community (#4969) fix(desktop): clamp thread panel to channel surface (#4965) style(messages): increase username contrast (#4948) fix(desktop): rename generic attachment action from 'Attach image' to 'Attach file' (#2381) (#4304) fix(reactions): support max-length custom emoji (#3833) feat(desktop): allow leaving your final community (#3621) fix(buzz-agent): recover from context-window 400s instead of sticking (#4946) docs(persona-pack): fix stale desktop import instructions (#4500) fix(desktop): route macos notification clicks (#4799) feat(mobile): sync themes per community (#3767) feat(desktop): sync themes per community (#3653) feat(desktop): cap OpenClaw agent parallelism at 5 (#4019) fix(buzz-agent): scope handoff cap per turn, not per session lifetime (#4805) Fix mobile message timeline bounce (#4862) Polish mobile bottom sheets and profile cards (#4911) Fix media attachment actions (#4849) fix(desktop): remove join API token control (#4897) ... Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz> # Conflicts: # desktop/src-tauri/src/commands/agents.rs # desktop/src-tauri/src/commands/agents_deploy.rs # desktop/src-tauri/src/managed_agents/env_vars.rs
Brings main (a7ea86c) into the PR branch as a merge commit — no rebase, no force — so the PR's merge ref is rebuilt against current main. Merged clean: no conflicts, and the resulting tree is byte-identical to `git merge-tree --write-tree` (fe05a2b). No manual conflict resolution was performed, so no observer-batching production or test byte changed in this commit. Note on CI: this merge does not by itself turn the board green. The two red Desktop Smoke shard-3 cases are main-branch defects that this branch inherits through the merge, both root-caused and reproduced on main at a7ea86c (which cannot contain this PR): - inbox-edit.spec.ts:233 expects the composer aria-label "Attach image", renamed to "Attach file" on main by d42d60d (#4304); the spec was added afterwards by eb6a375 (#2198), so the rename could not have updated it. Deterministic: 3/3 red on main in CI, 2/2 reproduced locally, 5/5 green with the one-word fix. - messaging.spec.ts:606 asserts a single day divider, but the e2e bridge seeds #general at now-120s and now-60s; within ~2 minutes of local midnight those straddle two calendar days and the non-.first() locator trips strict mode. Reproduced on main with page.clock.install pinned to 00:01:28Z (2 dividers) versus 12:00:00Z (1 divider). Both fixes belong in their own PR against main, not here. * origin/main: fix(desktop): enable the content security policy (#4614) fix(mobile): merge relay recounts with locally seen thread replies (#4633) fix(desktop): enable message editing in Inbox (#2198) relay: fuzz WebSocket 1012 restart-close timing on graceful drain (BUZZ_DRAIN_JITTER_MS) (#4542) fix(desktop): outline the selected community (#4969) fix(desktop): clamp thread panel to channel surface (#4965) style(messages): increase username contrast (#4948) fix(desktop): rename generic attachment action from 'Attach image' to 'Attach file' (#2381) (#4304) fix(reactions): support max-length custom emoji (#3833) feat(desktop): allow leaving your final community (#3621) fix(buzz-agent): recover from context-window 400s instead of sticking (#4946) docs(persona-pack): fix stale desktop import instructions (#4500) Co-authored-by: Sami <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@buzz.block.builderlab.xyz> Signed-off-by: Sami <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@buzz.block.builderlab.xyz>
…igration * origin/main: (95 commits) fix(desktop): virtualize channel member lists (#4991) fix(desktop): enforce owner-only access in internal builds (#4053) test(desktop): match attachment button label (#4993) fix(acp): pace observer telemetry at 1/s with per-channel batch envelopes (#4917) fix(desktop): enable the content security policy (#4614) fix(mobile): merge relay recounts with locally seen thread replies (#4633) fix(desktop): enable message editing in Inbox (#2198) relay: fuzz WebSocket 1012 restart-close timing on graceful drain (BUZZ_DRAIN_JITTER_MS) (#4542) fix(desktop): outline the selected community (#4969) fix(desktop): clamp thread panel to channel surface (#4965) style(messages): increase username contrast (#4948) fix(desktop): rename generic attachment action from 'Attach image' to 'Attach file' (#2381) (#4304) fix(reactions): support max-length custom emoji (#3833) feat(desktop): allow leaving your final community (#3621) fix(buzz-agent): recover from context-window 400s instead of sticking (#4946) docs(persona-pack): fix stale desktop import instructions (#4500) fix(desktop): route macos notification clicks (#4799) feat(mobile): sync themes per community (#3767) feat(desktop): sync themes per community (#3653) feat(desktop): cap OpenClaw agent parallelism at 5 (#4019) ... Signed-off-by: Brad Seiler <seiler@squareup.com>
2 participants
Summary
Why
On mobile, the "N replies" badge under a channel message can stall at a stale count or remain missing after a reply arrives. This makes the badge unreliable and can cause people to miss replies.
The badge has two inputs: best-effort recounts from the relay and replies the client sees arrive. Mobile previously let any positive relay recount override the local view, while also discarding replies from its local message store. A delayed or lost recount, or a reply received after the recount, could therefore leave the badge behind.
This change combines both inputs by using the higher reply count, the later last-reply time, and a merged participant list. Relay timestamps have one-second precision, so equal timestamps do not prove that a recount included a locally observed reply. Comparing counts preserves that reply instead of trusting recency alone. Desktop already uses this merge behavior.
Validation
At commit
4e3356636f5ad62e8f07910af305c532186c6c08with a clean worktree:flutter testfor mobile: 1105 passed, 1 skippedflutter analyzefor mobile: no issues foundAdded tests:
timeline_message_test.dart, covering relay-only recounts, a reply newer than the recount, a reply in the same second as the recount, a lost recount, a zero recount, nested replies at the root and at the reply they answer, a deleted reply, and participant merging and capping.channel_messages_provider_test.dart, covering a live reply reaching the store while staying out of the main timeline, and a reply newer than the relay recount raising the badge.